Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce Utils::getRandomFloat() #6532

Merged
merged 6 commits into from
Nov 26, 2024
Merged

Introduce Utils::getRandomFloat() #6532

merged 6 commits into from
Nov 26, 2024

Conversation

AkmalFairuz
Copy link
Contributor

Related issues & PRs

Changes

API changes

  • Added pocketmine\utils\Utils::getRandomFloat(float $min = 0.0, float $max = 1.0): float

Behavioural changes

Backwards compatibility

Follow-up

Tests

@AkmalFairuz AkmalFairuz requested a review from a team as a code owner November 24, 2024 11:43
dktapps
dktapps reviewed Nov 24, 2024
View reviewed changes
dktapps
dktapps reviewed Nov 24, 2024
View reviewed changes
dktapps
dktapps requested changes Nov 24, 2024
View reviewed changes
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would've been a lot more straightforward to review if you hadn't introduced unnecessary extra changes

@dktapps dktapps added Category: PHP Status: Waiting on Author Type: Enhancement Contributes features or other improvements to PocketMine-MP Multiple changes PR contains multiple changes and requires separation labels Nov 24, 2024
@AkmalFairuz
Copy link
Contributor Author

Would it be better if we implement Math::random(): float in pmmp/Math like Java and Javascript?

@dktapps
Copy link
Member

dktapps commented Nov 24, 2024

Well, really the randoms should be derived from a World-local Random object instead of on a process-global Random state, since the current way makes it basically impossible to reproduce random-driven events with a seed. However that's outside the scope of this PR.

Basically I don't think there's a need for modifying the Math library, although others may disagree.

dktapps
dktapps previously approved these changes Nov 24, 2024
View reviewed changes
@dktapps dktapps removed the Multiple changes PR contains multiple changes and requires separation label Nov 24, 2024
ipad54
ipad54 previously approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@ipad54 ipad54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to target minor-next

@dktapps dktapps changed the base branch from stable to minor-next November 25, 2024 14:40
@dktapps dktapps dismissed stale reviews from ipad54, ShockedPlot7560, and themself November 25, 2024 14:40

The base branch was changed.

@SOF3
Copy link
Member

SOF3 commented Nov 26, 2024

Basically I don't think there's a need for modifying the Math library, although others may disagree.

The Math library right now is more like a linalg/minecraft-coordinates library. I don't think it is a good idea to add random math-related functions into that library.

@dktapps dktapps merged commit 269effc into pmmp:minor-next Nov 26, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dktapps dktapps dktapps approved these changes

@ipad54 ipad54 ipad54 left review comments

@ShockedPlot7560 ShockedPlot7560 ShockedPlot7560 left review comments

Assignees
No one assigned
Labels
Category: PHP Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lcg_value() is deprecated in PHP 8.4
5 participants
@AkmalFairuz @dktapps @SOF3 @ipad54 @ShockedPlot7560